Skip to content

Remove @return(undefined_to_opt) and %undefined_to_opt primitive #7462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove @return(undefined_to_opt) and %undefined_to_opt primitive #7462

wants to merge 1 commit into from

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented May 11, 2025 •
edited
Loading 

@return(undefined_to_opt)
@send external get: (t<'k, 'v>, 'k) => option<'v> = "get"

produces compiler output like

let v = m.get("A");

let v$1 = v === undefined ? undefined : Primitive_option.some(v);

which doesn't make any sense. This is because it is a leftover from the days when options were compiled differently and None was not represented as undefined in the JS output yet.

As this feature was never documented, remove it.

Similarly, I think we can replace %undefined_to_opt by %identity. This leads to slight changes in the compiler output. @cristianoc could you have a look to check if everything is indeed fine?

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented May 11, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7462

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7462

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7462

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7462

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7462

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7462

commit: 2eddf54

@cknitt cknitt requested a review from cristianoc May 11, 2025 17:24
@cristianoc
Copy link
Collaborator

There's some behaviour change after this PR, which can be illustrated through array pop.

This pops an array with a single element and prints whether the resulting value is absent, or prints the value inside the option if present https://rescript-lang.org/try?version=v11.1.4&module=esmodule&code=DYUwLgBGIM5gCgewA4QLwQG7oHwCgIIYB3ASzAGMALCAKRgDoBBAJxYEMBPB5FACgDamALoBKCAG8CEAD4QAcogB2IXBADCymIlANgiAOZ8AROwBGMEErDHR0uQGVEAWxB8AHuLQ4NWnSD1DDztCAF88PE0lbV19I2MAHSUAKmSIFDBSZQAeUmsfVNs8aDgkZD5FFTsShH4nVz4AZlE7SL9YoMSUtIyspWzenLywHALkopqyvnq3SpAW4tha8pmmhaA

If we store in the array an element of option<int>, there's no difference before and after the PR. Notice in particular that popping [None] returns "absent", just like for []. So the presence of an element is not respected.

If we move to elements of option<option<int>>, popping [Some(None)] returns that a value is present, and the value is Some(None) before this PR, but None after this PR.

So the more legit case of storing a single optional value in the array does not behave in the expected way anyway.
And the pretty weird case of storing double options behaves differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristianoc cristianoc Awaiting requested review from cristianoc

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cknitt @cristianoc